-
-
Notifications
You must be signed in to change notification settings - Fork 420
BIRMINGHAM | 26-ITP-JAN | MERVE REIS | Sprint 2 | PRODUCT FORM #1079
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for cyf-onboarding-module ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
takanocap
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check that you have saved the assignment and pushed to github. I am not sure I can see any solution in the file
|
@takanocap I've added the product form to the html file. you can check again now |
Form-Controls/index.html
Outdated
| <meta charset="UTF-8" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
| <title>Product Form</title> | ||
| <link rel="stylesheet" href="style.css" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well done for the good Job. You have also included accessibility thats very good. Please keep it up. However, your styles.css may be missing or using the wrong reference. Please check this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@takanocap I removed unused style linking.
Form-Controls/index.html
Outdated
|
|
||
| <div> | ||
| <label for="email">E-mail</label> | ||
| <input type="email" name="E-mail" id="email" placeholder="name@example.com" required aria-required="true" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible to use the minlength for additional validation for email?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@takanocap I added min length feature.
Form-Controls/index.html
Outdated
|
|
||
| <div> | ||
| <label for="colour">Colour</label> | ||
| <select name="colour" id="colour" required aria-required="true" aria-label="Select a colour"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
welldone for the good job.
when we have a visible label could there be a reason why using aria-label will be redundant ? what is the difference between the aria-label and aria-labelledby, when should we use one or the other
would there be a reason why using aria-required and required may be redundant ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@takanocap Thanks for the feedback. Since there is already a visible label and the required attribute provides the necessary accessibility information, I’ve reverted the redundant aria-label and aria-required usage.
takanocap
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
welldone for the good job.
LGTM

Learners, PR Template
Self checklist
Changelist
Created a product pick form